Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib/filterx: add startswith / endswith functions #277

Closed
wants to merge 5 commits into from
Closed

lib/filterx: add startswith / endswith functions #277

wants to merge 5 commits into from

Conversation

OverOrion
Copy link
Contributor 

@version: 4.8

log {
  source {stdin(flags(no-parse));};
  filterx {
    declare needle = "bar";
    declare path = "";

    if (startswith($MSG, "foo", ignorecase=true)) {
      path = "starts with foo";
    }
    elif (startswith($MSG, needle)) {
      path = "starts with bar";
    }
    elif (endswith($MSG, "foo", ignorecase=true)) {
      path = "ends with foo";
    }
    elif (endswith($MSG, needle, ignorecase=true)) {
      path = "ends with bar";
    }
    else {path = "NOPE";};
  };
  filterx {vars();};
  destination { file("/dev/stdout"); };
};

@OverOrion OverOrion force-pushed the filterx-startswith-endswith branch from 0b96fb7 to e3bf29f Compare September 6, 2024 18:49
@OverOrion OverOrion force-pushed the filterx-startswith-endswith branch from e3bf29f to 2b0315b Compare September 6, 2024 18:52
@OverOrion OverOrion requested a review from alltilla September 9, 2024 06:01
alltilla
alltilla reviewed Sep 9, 2024
View reviewed changes
lib/filterx/func-str.c
@@ -33,6 +33,7 @@


#define FILTERX_FUNC_STARTSWITH_USAGE "Usage: startswith(my_string, my_prefix)"
#define FILTERX_FUNC_ENDSWITH_USAGE "Usage: endswith(my_string, my_prefix)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "my_suffix"

lib/filterx/func-str.c
FilterXExpr *needle_expr = filterx_function_args_get_expr(args, 1);
if (!filterx_expr_is_literal(needle_expr))
{
needle->expr = filterx_expr_ref(needle_expr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this ref is unnecessary.

lib/filterx/func-str.c
gsize needle_str_len;
const gchar *needle_str = filterx_function_args_get_literal_string(args, 1, &needle_str_len);
if (!needle_str)
return FALSE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please set an error message here.

lib/filterx/func-str.c
return FALSE;
}
if (!ignorecase)
*needle_str = g_strdup(needle_str_res);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that this strdup() is here because this way the function always returns a string that needs to be freed, but if ignorecase is not set, this is basically an unnecessary copy. Can we solve this somehow? Maybe we can return whether the returned string needs freeing or not. Or we could check for self->ignorecase on the call site to decide whether we need to free or not (the check could even be extracted to a function, but it might be overkill).

Same for haystack.

lib/filterx/func-str.c
filterx_expr_unref(self->haystack);
if (self->needle.expr)
filterx_expr_unref(self->needle.expr);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who frees self->needle.literal.str?

lib/filterx/func-str.h
Comment on lines +30 to +47
typedef struct _FilterXExprOrLiteral
{
FilterXExpr *expr;
struct
{
gchar *str;
gssize str_len;
} literal;
} FilterXExprOrLiteral;

typedef struct _FilterXFuncStartsWith
{
FilterXFunction super;
FilterXExpr *haystack;
FilterXExprOrLiteral needle;
gsize needle_len;
gboolean ignore_case;
} FilterXFuncStartsWith;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be "private" == defined in the .c file.

alltilla
alltilla reviewed Sep 9, 2024
View reviewed changes
tests/light/functional_tests/filterx/test_filterx.py
Comment on lines +2044 to +2076
def test_startswith_literal(config, syslog_ng):
(file_true, file_false) = create_config(
config, r"""
if (startswith($MSG, "foo"))
{
$MSG = json();
$MSG.startswith_foo = true;
};
""", msg="foo",
)
syslog_ng.start(config)

assert file_true.get_stats()["processed"] == 1
assert "processed" not in file_false.get_stats()
assert file_true.read_log() == '{"startswith_foo":true}\n'


def test_startswith_expr_ignorecase(config, syslog_ng):
(file_true, file_false) = create_config(
config, r"""
prefix = "FoO";
if (startswith($MSG, prefix, ignorecase=true))
{
$MSG = json();
$MSG.startswith_foo_ignorecase = true;
};
""", msg="foo",
)
syslog_ng.start(config)

assert file_true.get_stats()["processed"] == 1
assert "processed" not in file_false.get_stats()
assert file_true.read_log() == '{"startswith_foo_ignorecase":true}\n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional]

Having 2 (4 with endswith) testcases is cleaner, but takes longer time to execute, as we start syslog-ng multiple times, and it has an overhead.
Could you merge the 4 into one testcase, please?

@OverOrion OverOrion closed this Sep 19, 2024
@OverOrion OverOrion mentioned this pull request Sep 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alltilla alltilla alltilla left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@OverOrion @alltilla